Flow-editing follow-ups: Z-shape offset, attach-failure UX, serializer unification, dead-code removal#824
Merged
Merged
Conversation
A straight 2-point flow pinned between two stocks had no reroute path at all: the L-shape conversion required a cloud endpoint, and segment drags require interior segments. A perpendicular-dominant valve drag now inserts two corners at the dragged coordinate, keeping both endpoints pinned to their stock edges; the middle segment is then a normal interior segment (draggable, and dragging it back onto the axis collapses the flow to straight via normalizeFlowPoints). Stock moves preserve a Z's occupied edge: computeFlowRoute and getFlowAttachmentInfo share one side classifier (resolveStockAdjacentSide) that keeps an endpoint on its current edge when the stock-adjacent segment runs parallel to it (the Z riser), so nudges neither jump the endpoint to another edge nor collide spread slots with co-located flows. The valve clamps to the Z's middle segment specifically. LAYOUT.md's stale 20px valve-clearance prose is corrected to the actual VALVE_CLAMP_MARGIN (10px).
handleFlowAttach early-returned on patch failure without committing the view, silently discarding the flow the user just drew (Canvas had already cleared its in-creation staging on pointer-up). Worse, the selection was still set to the never-committed flow's uid, and handleEditingNameDone resolved that selection with the throwing getElementByUid -- the deferred tool-change editing-done then wedged the editor in a repeated-exception loop. The failure path now commits the optimistic view like handleCreateVariable and handleSelectionDelete already do, so the drawn flow stays selectable/renamable/deletable and the engine error surfaces via the toast. On a genuinely failing upsertFlow this persists a view flow with no backing variable; that divergence class is already produced by the sibling handlers, renders via the dangling-ref guards, is removable by keyboard delete, and self-heals on name-edit Escape. Defense in depth: handleEditingNameDone resolves the selection with tryGetElementByUid and tears down cleanly on a phantom or non-singleton selection, mirroring the render-time guard.
The StockFlowView -> JsonView serializer existed twice: the @simlin/core/datamodel implementation (used by model serialization and merge-live-view) and an independent copy in diagram/view-conversion.ts (used by Editor and project-controller). The same view could therefore serialize differently depending on code path, in exactly the save and live-view-merge paths recent flow-editing bugs went through. All callers now use the core implementation and the diagram copy is deleted. The two implementations were byte-identical for every already-correct case; the diagram copy's genuine divergences were all bugs, fixed by the unification: link polarity and the view's useLetteredPolarity flag were silently dropped from saves, a link carrying both arc and multiPoint emitted both (the engine treats them as alternatives, arc first), and a viewBox with one zero dimension was discarded. Each resolution matches the engine's json.rs semantics, pinned by new round-trip tests through the real deserializers, including the Z-shaped-flow unattached-corner case. The remaining view-fidelity gaps (ViewElementCompat, View.font, group is_mdl_view_marker, View.name) are dropped identically by both implementations in a different layer and stay tracked as issue 811.
adjustFlows carried two deliberately-different valve-fraction formulas selected by an optional isCloud parameter, but its single caller (UpdateCloudAndFlow's straight-flow parallel-drag path) always passed true, so the signed non-cloud formula could never execute. The cloud formula is now inlined and the parameter removed; the surviving comment keeps the load-bearing rationale (mirror the valve around the segment with an absolute scaled offset so an off-center valve stays between the ends when a drag flips segment orientation, plus the zero-denominator guard). Behavior-preserving: the inlined body is character-identical to the old isCloud branch, and the pre-existing characterization tests (off-center, centered, zero-delta, non-finite guard, degenerate self-loop) pass unchanged.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #824 +/- ##
=======================================
Coverage 90.87% 90.87%
=======================================
Files 224 224
Lines 143133 143133
=======================================
+ Hits 130071 130072 +1
+ Misses 13062 13061 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Reviewed the four commits end-to-end and traced through the routing math, the Editor/Canvas defensive changes, and the serializer unification. No material findings. Nits I looked at but decided against flagging:
Overall verdict: correct. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Four follow-ups from the June 2026 flow-editing audit, one commit each. Every fix was implemented by one agent and adversarially reviewed by a separate fresh agent, iterating until no material findings remained; interactive behavior was additionally verified live in the browser against the dev server.
Fixes #819
Fixes #820
Fixes #821
Fixes #822
diagram: offset stock-to-stock flows into a Z on perpendicular drag (#819)
A straight flow pinned between two stocks had no reroute path at all (the L-shape conversion required a cloud endpoint; segment drags require interior segments). A perpendicular-dominant valve drag now inserts two corners to form a Z; the middle segment is a normal interior segment, so dragging it back onto the axis collapses the flow to straight via the existing normalization.
Non-obvious decisions surfaced by review and fixed in the same commit: stock moves preserve a Z's occupied edge via a single shared side classifier (
resolveStockAdjacentSide) used by bothcomputeFlowRouteandgetFlowAttachmentInfo-- without it the endpoint jumped edges on a 1px nudge, and a Z sharing a stock edge with a straight flow collided at the identical attachment point. The valve clamps to the Z's middle segment specifically. LAYOUT.md's valve-clearance prose said 20px; the code has always usedVALVE_CLAMP_MARGIN(10px), so the doc was corrected rather than the behavior changed.diagram: preserve the drawn flow when a flow-attach patch fails (#820)
handleFlowAttachearly-returned on patch failure without committing the view, silently discarding the flow the user just drew -- and leaving the selection pointing at a flow that was never committed, which the name-edit teardown then dereferenced with a throwing lookup, wedging the editor in an exception loop (this crash is what made the editor look broken during live audit sessions). The failure path now commits the optimistic view exactly like the sibling create/delete handlers already do, so the drawn flow stays selectable, renamable, and deletable while the engine error surfaces via the toast;handleEditingNameDoneadditionally resolves the selection non-throwing and tears down cleanly on a phantom selection.Review adjudicated preserve-vs-discard with engine-level evidence: the resulting view-only flow is the same divergence class the sibling handlers already produce, renders through the existing dangling-ref guards, is removable via keyboard delete, self-heals on name-edit Escape, and undoes as a single history entry.
core: unify stockFlowViewToJson, delete the diagram-local copy (#821)
The view serializer existed twice and diagram callers were split between the copies, so the same view could serialize differently on the save path vs the live-view-merge path. All callers now use the
@simlin/coreimplementation. A differential test across every element type showed the copies were byte-identical for all correct cases; the diagram copy's divergences were bugs, so unification also recovers silently dropped save data: linkpolarity, the view'suseLetteredPolarityflag, arc-over-multiPoint precedence, and one-zero-dimension viewBoxes. Each resolution was verified against the engine's json.rs semantics and pinned with round-trip tests through the real deserializers. The remaining view-fidelity gaps are dropped identically by both implementations in a different layer and stay tracked as #811.diagram: remove the dead non-cloud branch from adjustFlows (#822)
The single caller always passed
isCloud=true, so the signed non-cloud valve formula could never execute. The cloud formula is inlined and the parameter removed; review confirmed the surviving body is character-identical to the old live branch and the pre-existing characterization tests pass unchanged.